apps/ui/src/components/views/board-view/components/kanban-card/card-header.tsx-129-161 (1)

129-161: ⚠️ Potential issue | 🟡 Minor

Update test IDs to reflect actual feature status instead of hardcoding "backlog".

The condition now includes interrupted and ready statuses, but data-testid values remain hardcoded as spawn-backlog- and delete-backlog-. This creates misleading test identifiers when the feature status is interrupted or ready.

Use feature.status to derive the test ID:

Proposed fix

-              data-testid={`spawn-backlog-${feature.id}`}
+              data-testid={`spawn-${feature.status}-${feature.id}`}
               title="Spawn Sub-Task"
             >
               <GitFork className="w-4 h-4" />
             </Button>
             <Button
               variant="ghost"
               size="sm"
               className="h-6 w-6 p-0 hover:bg-white/10 text-muted-foreground hover:text-destructive"
               onClick={handleDeleteClick}
               onPointerDown={(e) => e.stopPropagation()}
-              data-testid={`delete-backlog-${feature.id}`}
+              data-testid={`delete-${feature.status}-${feature.id}`}

.planning/codebase/STACK.md-108-108 (1)

108-108: ⚠️ Potential issue | 🟡 Minor

Ambiguous zod version.

"zod 3.24.1 or 4.0.0" — these are very different major versions with breaking API changes (zod 4 has a different import structure). Clarify which version is actually in use.

.gitignore-98-100 (1)

98-100: ⚠️ Potential issue | 🟡 Minor

.planning/ is gitignored but also committed in this PR.

The PR adds .planning/ to .gitignore while simultaneously committing multiple files under .planning/ (STACK.md, CONVENTIONS.md, PROJECT.md, CONCERNS.md, etc.). Once tracked, existing files remain tracked despite the gitignore entry, but this creates confusion: future edits to these files will appear in git status, yet new files under .planning/ will be silently ignored. If the intent is to keep these docs in the repo, remove this gitignore entry. If they should be local-only, don't commit them.

.planning/codebase/CONCERNS.md-47-48 (1)

47-48: ⚠️ Potential issue | 🟡 Minor

Stale file references to auto-mode-service.ts.

Multiple sections reference specific lines in apps/server/src/services/auto-mode-service.ts (lines 47–48, 93, 129), but this PR deletes and decomposes that monolith. These references will be immediately stale. Update them to point to the new service files (e.g., recovery-service.ts, pipeline-orchestrator.ts, auto-loop-coordinator.ts).

Also applies to: 93-93, 129-129

.planning/codebase/CONVENTIONS.md-9-9 (1)

9-9: ⚠️ Potential issue | 🟡 Minor

Documentation error: examples contradict the stated convention.

Line states "PascalCase for class/service files" but all examples (auto-mode-service.ts, feature-loader.ts, claude-provider.ts) are kebab-case. The convention appears to be kebab-case for all files; fix the label.

Proposed fix

-- PascalCase for class/service files: `auto-mode-service.ts`, `feature-loader.ts`, `claude-provider.ts`
+- kebab-case for class/service files: `auto-mode-service.ts`, `feature-loader.ts`, `claude-provider.ts`

.planning/codebase/INTEGRATIONS.md-108-108 (1)

108-108: ⚠️ Potential issue | 🟡 Minor

Inconsistency with ARCHITECTURE.md on log storage.

Line 108 states "(no persistent logging to files)" but ARCHITECTURE.md line 204 states "Output: stdout (dev), files (production)". These docs are being added in the same PR — align them on the actual behavior.

.planning/codebase/STRUCTURE.md-233-233 (1)

233-233: ⚠️ Potential issue | 🟡 Minor

Contradictory naming convention: examples don't match the stated convention.

Line 233 states "Components: PascalCase.tsx" but the examples (board-view.tsx, session-manager.tsx) are kebab-case, not PascalCase. Either correct the convention label to "kebab-case" or update the examples.

.planning/codebase/STRUCTURE.md-238-238 (1)

238-238: ⚠️ Potential issue | 🟡 Minor

Markdown rendering issue with wildcard patterns.

_.test.ts or _.spec.ts — the asterisks are being interpreted as emphasis markers (flagged by MD037). Use backtick escaping:

Proposed fix

-- **Tests:** _.test.ts or _.spec.ts (co-located with source)
+- **Tests:** `*.test.ts` or `*.spec.ts` (co-located with source)

apps/server/src/services/recovery-service.ts-122-146 (1)

122-146: ⚠️ Potential issue | 🟡 Minor

saveExecutionState (legacy) captures running features across all projects.

Unlike saveExecutionStateForProject (Line 101) which filters by projectPath, the legacy saveExecutionState at Line 135 calls getAllRunning().map(...) without filtering. This means runningFeatureIds will include features from other projects.

If this is a known limitation of the legacy API that's being phased out, a brief comment would help. Otherwise, it should filter to match the new method's behavior.

apps/server/tests/unit/services/feature-state-manager.test.ts-266-285 (1)

266-285: ⚠️ Potential issue | 🟡 Minor

Tests for "preserve pipeline_ statuses" don't actually exercise the intended code path.*

These tests mock secureFs.readFile but markFeatureInterrupted calls this.loadFeature() which uses readJsonWithRecovery (not secureFs.readFile). Since readJsonWithRecovery is a bare vi.fn() mock (returns undefined), loadFeature will throw internally and return null. The currentStatus check at the pipeline_* guard never fires — instead, updateFeatureStatus is called but also silently returns because it also reads null from the same bare mock.

The assertion expect(atomicWriteJson).not.toHaveBeenCalled() passes, but for the wrong reason (feature not found), not because the pipeline status was preserved.

💚 Proposed fix — mock readJsonWithRecovery to return the pipeline feature

   it('should preserve pipeline_* statuses', async () => {
-    (secureFs.readFile as Mock).mockResolvedValue(
-      JSON.stringify({ ...mockFeature, status: 'pipeline_step_1' })
-    );
+    (readJsonWithRecovery as Mock).mockResolvedValue({
+      data: { ...mockFeature, status: 'pipeline_step_1' },
+      recovered: false,
+      source: 'main',
+    });

     await manager.markFeatureInterrupted('/project', 'feature-123', 'server shutdown');

     // Should NOT call atomicWriteJson because pipeline status is preserved
     expect(atomicWriteJson).not.toHaveBeenCalled();
   });

   it('should preserve pipeline_complete status', async () => {
-    (secureFs.readFile as Mock).mockResolvedValue(
-      JSON.stringify({ ...mockFeature, status: 'pipeline_complete' })
-    );
+    (readJsonWithRecovery as Mock).mockResolvedValue({
+      data: { ...mockFeature, status: 'pipeline_complete' },
+      recovered: false,
+      source: 'main',
+    });

     await manager.markFeatureInterrupted('/project', 'feature-123');

     expect(atomicWriteJson).not.toHaveBeenCalled();
   });

apps/server/src/services/execution-types.ts-19-37 (1)

19-37: ⚠️ Potential issue | 🟡 Minor

projectPath appears as both a required positional parameter and an optional field in options.

RunAgentFn accepts projectPath at position 5 (Line 24) and also as options.projectPath (Line 28). This duplication is confusing for callers — which one takes precedence? All current implementations pass the same value to both, but this redundancy creates ambiguity. Remove the optional projectPath from options to keep a single source of truth.

.planning/codebase/TESTING.md-48-69 (1)

48-69: ⚠️ Potential issue | 🟡 Minor

Add language identifier to fenced code block.

The directory tree structure block on line 48 lacks a language specifier. This triggers the markdownlint MD040 warning. Use text or plaintext for non-code content.

📝 Proposed fix

-```
+```text
 apps/server/

apps/ui/src/components/views/board-view/hooks/use-board-drag-drop.ts-37-37 (1)

37-37: ⚠️ Potential issue | 🟡 Minor

autoMode in dependency array defeats useCallback memoization.

useAutoMode() returns a new object reference on every render (the return object is not memoized, only the internal functions are). Including autoMode in the useCallback dependencies means handleDragEnd is recreated every render despite being wrapped in useCallback, which can cause unnecessary re-renders of downstream @dnd-kit components that depend on handler identity.

Since only stopFeature is used from autoMode in handleDragEnd, destructure it instead:

-  const autoMode = useAutoMode();
+  const { stopFeature } = useAutoMode();

Then update the call on line 233 from autoMode.stopFeature(featureId) to stopFeature(featureId), and replace autoMode with stopFeature in the dependency array (line 328). This preserves memoization since stopFeature is itself wrapped in useCallback inside the hook.

apps/ui/src/hooks/use-auto-mode.ts-614-626 (1)

614-626: ⚠️ Potential issue | 🟡 Minor

refreshStatus is not in the dependency array of start (and stop).

refreshStatus is captured by the queueMicrotask closure but isn't listed as a dependency of the start callback (line 626). If refreshStatus is recreated (e.g., because runningAutoTasks changes — now a dependency at line 197), the queued call could use a stale reference.

The impact is minor since the next poll cycle will correct it, but it's technically a stale-closure risk. Same applies to stop at line 674.

apps/server/tests/unit/services/execution-service.test.ts-364-394 (1)

364-394: ⚠️ Potential issue | 🟡 Minor

Test name says "throws" but the method doesn't throw — it emits an error event.

The test "throws if feature not found" (line 364) calls svc.executeFeature without await expect(...).rejects. The method handles the error internally and emits an auto_mode_error event. The test name should reflect this behavior.

♻️ Suggested rename

-   it('throws if feature not found', async () => {
+   it('emits error event when feature not found', async () => {

apps/ui/src/hooks/use-auto-mode.ts-15-19 (1)

15-19: ⚠️ Potential issue | 🟡 Minor

arraysEqual gives false positives when a contains duplicates.

The Set-based comparison only checks membership, not multiplicity. For example, arraysEqual(['a', 'a'], ['a', 'b']) returns true (both length 2, every element of a is in Set(['a','b'])).

In practice feature IDs are unique, so this is unlikely to cause real issues, but a simple sorted-comparison or symmetric set check would be safer.

♻️ Suggested fix

 function arraysEqual(a: string[], b: string[]): boolean {
   if (a.length !== b.length) return false;
-  const set = new Set(b);
-  return a.every((id) => set.has(id));
+  const sorted_a = [...a].sort();
+  const sorted_b = [...b].sort();
+  return sorted_a.every((id, i) => id === sorted_b[i]);
 }

apps/server/tests/unit/services/agent-executor.test.ts-395-440 (1)

395-440: ⚠️ Potential issue | 🟡 Minor

Test name is misleading — doesn't actually assert aborted=true.

The test is named "should return aborted=true when abort signal is already aborted" but only asserts result and result.responseText are defined. It never checks result.aborted === true. Either rename the test to match what it verifies, or add the missing assertion.

💚 Proposed fix

       // When stream is empty and signal is aborted before stream starts,
       // the result depends on whether abort was checked
       expect(result).toBeDefined();
       expect(result.responseText).toBeDefined();
+      expect(result.aborted).toBe(true);
     });

apps/server/src/services/auto-loop-coordinator.ts-275-310 (1)

275-310: ⚠️ Potential issue | 🟡 Minor

Non-null assertion errorInfo! is unsafe for certain call patterns.

At line 294, actualErrorInfo = errorInfo! assumes errorInfo is defined when branchNameOrError is a string or null. While TypeScript callers will generally provide it, this overloaded-via-runtime-check pattern is fragile. If a caller passes (projectPath, null) without the third argument, actualErrorInfo is undefined and line 301 throws at runtime.

Consider adding a defensive check or using proper function overloads.

🛡️ Suggested defensive check

    } else {
      // New signature: (projectPath, branchName, errorInfo)
      branchName = branchNameOrError;
-     actualErrorInfo = errorInfo!;
+     if (!errorInfo) {
+       logger.warn('trackFailureAndCheckPauseForProject called without errorInfo');
+       return false;
+     }
+     actualErrorInfo = errorInfo;
    }

apps/server/src/services/pipeline-orchestrator.ts-454-476 (1)

454-476: ⚠️ Potential issue | 🟡 Minor

Clear the timeout when the test run finishes.

When a session completes, the 10‑minute timeout still fires later. Clearing it prevents an unnecessary dangling timer.

🛠 Suggested fix

   private async waitForTestCompletion(
     sessionId: string
   ): Promise<{ status: TestRunStatus; exitCode: number | null; duration: number }> {
     return new Promise((resolve) => {
-      const checkInterval = setInterval(() => {
+      const timeout = setTimeout(() => {
+        clearInterval(checkInterval);
+        resolve({ status: 'failed', exitCode: null, duration: 600000 });
+      }, 600000);
+
+      const checkInterval = setInterval(() => {
         const session = this.testRunnerService.getSession(sessionId);
         if (session && session.status !== 'running' && session.status !== 'pending') {
           clearInterval(checkInterval);
+          clearTimeout(timeout);
           resolve({
             status: session.status,
             exitCode: session.exitCode,
             duration: session.finishedAt
               ? session.finishedAt.getTime() - session.startedAt.getTime()
               : 0,
           });
         }
       }, 1000);
-      setTimeout(() => {
-        clearInterval(checkInterval);
-        resolve({ status: 'failed', exitCode: null, duration: 600000 });
-      }, 600000);
     });
   }

apps/ui/src/components/views/board-view/components/kanban-card/card-actions.tsx (1)

1-1: @ts-nocheck disables all type safety for this file.

The comment mentions "optional callback prop typing with feature status narrowing" — but CardActionsProps already types the optional callbacks with ?. The status narrowing could be handled with discriminated unions or simple type guards. Blanket @ts-nocheck masks any future type regressions introduced in this file.

Consider removing it and fixing the specific type issues, or at minimum narrowing to targeted @ts-expect-error comments on the offending lines.

apps/ui/src/components/views/board-view/components/kanban-card/card-header.tsx (1)

1-1: @ts-nocheck suppresses all type safety for this file.

This directive hides potential type errors introduced by these or future changes. Consider removing it and fixing any underlying type issues, or at minimum adding a tracking issue to address it later.

package.json (1)

75-75: Inconsistent version pinning for lint-staged.

Changed from exact "16.2.7" to caret "^16.2.7", while sibling devDependencies (husky, prettier, vitest) use exact versions. Consider keeping exact pinning for consistency and reproducible builds.

Proposed fix

-    "lint-staged": "^16.2.7",
+    "lint-staged": "16.2.7",

.planning/codebase/CONCERNS.md (1)

53-56: Capitalize "GitHub" in theme names.

Lines 53 and 55 reference "github" terminal theme — should be "GitHub" when referring to the platform name in prose (per static analysis hint).

.planning/codebase/ARCHITECTURE.md (1)

39-39: Consider updating service list to reflect the decomposition.

Line 39 lists AutoModeService as a single service, but this PR decomposes it into AutoModeServiceCompat, AutoModeServiceFacade, GlobalAutoModeService, and several sub-services. Consider updating this list to reflect the new architecture, since this doc is being added in the same PR.

apps/server/src/services/worktree-resolver.ts (1)

65-102: Duplicated porcelain parsing logic between findWorktreeForBranch and listWorktrees.

Both methods parse git worktree list --porcelain output with nearly identical line-by-line iteration. findWorktreeForBranch could delegate to listWorktrees and filter by branch, eliminating ~35 lines of duplication.

Suggested refactor

  async findWorktreeForBranch(projectPath: string, branchName: string): Promise<string | null> {
-   try {
-     const { stdout } = await execAsync('git worktree list --porcelain', {
-       cwd: projectPath,
-     });
-
-     const lines = stdout.split('\n');
-     let currentPath: string | null = null;
-     let currentBranch: string | null = null;
-
-     for (const line of lines) {
-       if (line.startsWith('worktree ')) {
-         currentPath = line.slice(9);
-       } else if (line.startsWith('branch ')) {
-         currentBranch = line.slice(7).replace('refs/heads/', '');
-       } else if (line === '' && currentPath && currentBranch) {
-         if (currentBranch === branchName) {
-           return this.resolvePath(projectPath, currentPath);
-         }
-         currentPath = null;
-         currentBranch = null;
-       }
-     }
-
-     if (currentPath && currentBranch && currentBranch === branchName) {
-       return this.resolvePath(projectPath, currentPath);
-     }
-
-     return null;
-   } catch {
-     return null;
-   }
+   const worktrees = await this.listWorktrees(projectPath);
+   const match = worktrees.find(wt => wt.branch === branchName);
+   return match?.path ?? null;
  }

apps/server/tests/unit/services/auto-loop-coordinator.test.ts (1)

2-13: Consider using path alias for consistency with other test files.

Other test files in this PR (e.g., recovery-service.test.ts) use the @/ path alias for server-internal imports. This file uses deep relative paths (../../../src/services/...). Consider aligning for consistency.

🔧 Suggested import alignment

-import {
-  AutoLoopCoordinator,
-  getWorktreeAutoLoopKey,
-  type AutoModeConfig,
-  type ProjectAutoLoopState,
-  type ExecuteFeatureFn,
-  type LoadPendingFeaturesFn,
-  type SaveExecutionStateFn,
-  type ClearExecutionStateFn,
-  type ResetStuckFeaturesFn,
-  type IsFeatureFinishedFn,
-} from '../../../src/services/auto-loop-coordinator.js';
-import type { TypedEventBus } from '../../../src/services/typed-event-bus.js';
-import type { ConcurrencyManager } from '../../../src/services/concurrency-manager.js';
-import type { SettingsService } from '../../../src/services/settings-service.js';
+import {
+  AutoLoopCoordinator,
+  getWorktreeAutoLoopKey,
+  type AutoModeConfig,
+  type ProjectAutoLoopState,
+  type ExecuteFeatureFn,
+  type LoadPendingFeaturesFn,
+  type SaveExecutionStateFn,
+  type ClearExecutionStateFn,
+  type ResetStuckFeaturesFn,
+  type IsFeatureFinishedFn,
+} from '@/services/auto-loop-coordinator.js';
+import type { TypedEventBus } from '@/services/typed-event-bus.js';
+import type { ConcurrencyManager } from '@/services/concurrency-manager.js';
+import type { SettingsService } from '@/services/settings-service.js';

apps/server/src/services/merge-service.ts (1)

36-56: execGitCommand and isValidBranchName are duplicated from routes/worktree/common.ts.

Both helpers exist in apps/server/src/routes/worktree/common.ts with nearly identical implementations (the only difference is isValidBranchName uses MAX_BRANCH_NAME_LENGTH there vs. 250 here). Consider importing from the shared location or extracting to a common utility to avoid drift.

apps/server/src/services/feature-state-manager.ts (1)

168-192: markFeatureInterrupted silently returns when feature is not found.

When loadFeature returns null (Line 174), currentStatus is undefined, the pipeline_* check passes through, and updateFeatureStatus is called which also returns silently on null feature (Line 100). This is defensive but consider logging a warning at the markFeatureInterrupted level when the feature can't be found, to aid debugging crash-recovery issues.

apps/server/tests/unit/services/feature-state-manager.test.ts (1)

248-264: Same mock issue in "should mark feature as interrupted" test.

Line 250 mocks secureFs.readFile but the actual code path goes through readJsonWithRecovery. Lines 253–257 correctly mock readJsonWithRecovery for this test, but the secureFs.readFile mock on Line 250 is dead code. It won't cause failures but adds confusion about which mock is actually exercised.

🧹 Remove dead mock

   it('should mark feature as interrupted', async () => {
-    (secureFs.readFile as Mock).mockResolvedValue(
-      JSON.stringify({ ...mockFeature, status: 'in_progress' })
-    );
     (readJsonWithRecovery as Mock).mockResolvedValue({
       data: { ...mockFeature, status: 'in_progress' },
       recovered: false,
       source: 'main',
     });

apps/server/src/services/spec-parser.ts (1)

176-227: Local truncate shadows a utility in @automaker/utils.

The local truncate helper (Line 178) has different semantics from the one in libs/utils/src/string-utils.ts — it extracts the first paragraph and then truncates, which is purpose-specific to summary extraction. Consider renaming it (e.g., truncateFirstParagraph) to avoid confusion with the shared utility.

apps/server/src/services/recovery-service.ts (2)

78-90: Consider a deps/config object for the 11-parameter constructor.

The constructor has 11 parameters which makes call-site readability and future extensibility challenging. A common pattern is to accept a single options/deps object.

♻️ Example deps-object pattern

interface RecoveryServiceDeps {
  eventBus: TypedEventBus;
  concurrencyManager: ConcurrencyManager;
  settingsService: SettingsService | null;
  executeFeatureFn: ExecuteFeatureFn;
  loadFeatureFn: LoadFeatureFn;
  detectPipelineStatusFn: DetectPipelineStatusFn;
  resumePipelineFn: ResumePipelineFn;
  isFeatureRunningFn: IsFeatureRunningFn;
  acquireRunningFeatureFn: AcquireRunningFeatureFn;
  releaseRunningFeatureFn: ReleaseRunningFeatureFn;
}

export class RecoveryService {
  constructor(private deps: RecoveryServiceDeps) {}
  // access as this.deps.eventBus, etc.
}

---

250-301: Sequential resumption of interrupted features may be slow with many features.

resumeInterruptedFeatures awaits each resumeFeature call sequentially (Line 293). If there are many interrupted features, this blocks progress on later features while earlier ones complete. Consider firing and tracking promises concurrently (up to maxConcurrency), or at minimum fire-and-forget with error logging, since each resumeFeature is self-contained.

apps/server/src/services/plan-approval-service.ts (1)

306-312: planApprovalTimeoutMs accessed via as any cast — consider adding it to the ProjectSettings type.

The eslint-disable + as any here silently bypasses type safety. If ProjectSettings is under your control, adding planApprovalTimeoutMs?: number to that interface would be cleaner and would ensure downstream consumers know this setting exists.

apps/server/tests/unit/services/plan-approval-service.test.ts (1)

42-43: Unused flushPromises helper.

flushPromises is defined but never called — all tests use vi.advanceTimersByTimeAsync(0) directly instead. Consider removing it to avoid confusion.

apps/server/src/services/execution-types.ts (1)

89-94: _calledInternally parameter with underscore prefix in a type definition.

The underscore prefix convention signals an unused parameter at the implementation site, but embedding that convention in the type definition leaks implementation detail. Consumers implementing this type see _calledInternally and may be confused about whether it's intentionally ignored. Consider naming it calledInternally in the type and letting implementers use the underscore if they choose.

Same applies to ExecuteFeatureFn at Line 143.

apps/server/src/services/ideation-service.ts (1)

233-235: Fix looks correct for custom provider model handling.

When a custom provider (e.g., GLM, MiniMax) is detected, using options.model directly avoids resolving to a Claude model name that the custom endpoint wouldn't recognize. The default path (no custom provider) still uses the resolved model from line 219.

Note: the coding guideline states to always use resolveModelString() before API calls. Here it's intentionally bypassed for custom providers — the CRITICAL comment documents why. Consider extracting this into a small helper (e.g., resolveModelForProvider) that encapsulates this conditional logic, so the intent is clear without relying solely on comments.

apps/server/tests/unit/services/typed-event-bus.test.ts (1)

134-164: Event type list is incomplete relative to AutoModeEventType definition.

The "all standard auto-mode event types" test on Lines 134-152 is missing several types defined in typed-event-bus.ts: auto_mode_task_status, auto_mode_phase_complete, auto_mode_summary, auto_mode_resuming_features, plan_auto_approved, plan_revision_requested, plan_revision_warning, pipeline_step_started, pipeline_step_complete, and auto_mode_phase. Not a bug (since the | string catch-all ensures they work), but the test name "should handle all standard auto-mode event types" is misleading. Consider either completing the list or renaming the test.

apps/server/src/services/typed-event-bus.ts (1)

19-45: The | string catch-all negates type safety from the literal union.

The explicit event type literals serve as documentation but provide no compile-time safety since | string widens the type to accept any string. If extensibility is needed, consider a branded/template-literal pattern or removing | string and adding new event types explicitly as they're introduced. This would catch typos like passing 'auto_mode_strated' instead of 'auto_mode_started'.

Also, 'auto_mode_phase' (used in the UI's SINGLE_FEATURE_INVALIDATION_EVENTS) is missing from the explicit list, though it's covered by | string.

apps/server/src/services/auto-mode/global-service.ts (3)

62-67: Good safety guard, but consider logging when it fires.

The throwing executeFeatureFn is a solid defensive measure. However, if this ever fires unexpectedly due to a wiring bug, the generic Error might be caught and logged far from here without clear context.

Consider adding a logger.error(...) before the throw so it's easy to trace in logs.

---

105-112: getSharedServices() exposes the monitoring-only autoLoopCoordinator.

The returned autoLoopCoordinator has a throwing executeFeatureFn. Consumers of SharedServices must know not to use it for execution. The facade code (in facade.ts) correctly creates its own coordinator, but the type system doesn't enforce this — any caller with SharedServices could accidentally call execution methods on it.

Consider either:

1. Omitting autoLoopCoordinator from SharedServices (facades create their own anyway), or
2. Adding a prominent JSDoc warning on the SharedServices.autoLoopCoordinator type.

---

195-207: Sequential markFeatureInterrupted calls during shutdown.

The for...of with await processes features one at a time. During graceful shutdown with many running features, this could be slow. Using Promise.allSettled would be faster and still resilient to individual failures.

♻️ Suggested refactor

  async markAllRunningFeaturesInterrupted(reason?: string): Promise<void> {
    const allRunning = this.concurrencyManager.getAllRunning();

-   for (const rf of allRunning) {
-     await this.featureStateManager.markFeatureInterrupted(rf.projectPath, rf.featureId, reason);
-   }
+   await Promise.allSettled(
+     allRunning.map((rf) =>
+       this.featureStateManager.markFeatureInterrupted(rf.projectPath, rf.featureId, reason)
+     )
+   );

    if (allRunning.length > 0) {
      logger.info(
        `Marked ${allRunning.length} running feature(s) as interrupted: ${reason || 'no reason provided'}`
      );
    }
  }

apps/server/tests/unit/services/agent-executor.test.ts (2)

268-281: Function.length is fragile for verifying method signatures.

executor.execute.length only counts parameters before the first one with a default value. If a default is added later, this test silently becomes wrong. Consider removing this or documenting the caveat.

---

90-119: Constructor dependency tests don't verify storage — effectively duplicates.

Tests at lines 90-119 ("should store eventBus dependency", "should store featureStateManager dependency", "should store planApprovalService dependency") all just assert executor is toBeDefined(), identical to the first constructor test. They don't verify the dependency is actually stored or accessible. Consider either removing them or testing via observable behavior (e.g., verifying eventBus is called during execute).

apps/server/src/services/auto-loop-coordinator.ts (1)

181-192: Fire-and-forget executeFeatureFn — intentional but worth noting.

The executeFeatureFn at line 181 is called without await, relying solely on .catch() for error handling. This is intentional for concurrency (the loop should proceed to start more features). The .catch() handler logs and checks for pause conditions.

However, if .catch() itself throws (unlikely but possible if trackFailureAndCheckPauseForProject or signalShouldPauseForProject throws), it becomes an unhandled rejection. Consider wrapping the .catch() body in a try-catch.

apps/server/src/services/agent-executor.ts (2)

336-336: || 100 treats maxTurns: 0 as falsy — use ?? for nullish coalescing.

Math.min(sdkOptions?.maxTurns || 100, 50) — if maxTurns is explicitly set to 0, || falls through to 100, then caps at 50. While 0 maxTurns is likely not a valid use case, ?? is more correct:

-      this.buildExecOpts(options, taskPrompt, Math.min(sdkOptions?.maxTurns || 100, 50))
+      this.buildExecOpts(options, taskPrompt, Math.min(sdkOptions?.maxTurns ?? 100, 50))

---

126-145: Raw output uses pretty-printed JSON in a JSONL file.

JSON.stringify({ ... }, null, 4) produces multi-line JSON. Since the file extension is .jsonl (JSON Lines), each entry should be a single line. This makes the file unparseable with standard JSONL tools.

Since this is debug-only output (gated by AUTOMAKER_DEBUG_RAW_OUTPUT), it's low impact, but worth fixing for correctness.

♻️ Suggested fix

        rawOutputLines.push(
-         JSON.stringify({ timestamp: new Date().toISOString(), event }, null, 4)
+         JSON.stringify({ timestamp: new Date().toISOString(), event })
        );

apps/server/tests/unit/services/execution-service.test.ts (1)

170-262: Consider a helper factory to reduce repeated ExecutionService construction.

Many tests (lines 366-385, 481-500, 525-544, 574-593, etc.) re-create ExecutionService with 16+ constructor arguments when only one mock differs. A factory helper with overridable defaults would significantly reduce boilerplate.

♻️ Example helper

function createService(overrides: Partial<{
  loadFeatureFn: LoadFeatureFn;
  runAgentFn: RunAgentFn;
  contextExistsFn: ContextExistsFn;
  trackFailureFn: TrackFailureFn;
  // ... other overridable fns
}> = {}) {
  return new ExecutionService(
    mockEventBus,
    mockConcurrencyManager,
    mockWorktreeResolver,
    mockSettingsService,
    overrides.runAgentFn ?? mockRunAgentFn,
    mockExecutePipelineFn,
    mockUpdateFeatureStatusFn,
    overrides.loadFeatureFn ?? mockLoadFeatureFn,
    // ... etc
  );
}

apps/server/src/services/concurrency-manager.ts (1)

64-66: Consider a safer default for getCurrentBranch.

Defaulting to 'main' can miscount worktree capacity in repos with master or custom primary branches when the dependency isn’t injected. A null default avoids misclassification.

🛠 Suggested tweak

-    this.getCurrentBranch = getCurrentBranch ?? (() => Promise.resolve('main'));
+    this.getCurrentBranch = getCurrentBranch ?? (() => Promise.resolve(null));

🤖 Fix all nitpicks with AI agents

Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.

In `@apps/server/src/services/auto-mode/facade.ts`:
- Around line 167-238: The anonymous async runAgentFn bodies contain duplicated
provider resolution, credential lookup and agentExecutor.execute invocation
(including buildTaskPrompt logic); extract shared helpers inside create such as
a getResolvedProvider(model) helper that uses
ProviderFactory.getProviderForModel, stripProviderPrefix and
getProviderByModelId to return {provider, effectiveBareModel, credentials,
claudeCompatibleProvider}, and extract a buildTaskPrompt(task, allTasks,
taskIndex, template, feedback) helper to centralize the template replacements;
then replace both inline blocks (the async runAgentFn passed to
PipelineOrchestrator and the one passed to ExecutionService) to call these
helpers and invoke agentExecutor.execute with the unified parameter object and
callbacks.
- Around line 660-669: The loop in verifyFeature currently uses
execAsync(check.cmd) which is inconsistent with the spawnProcess/execGitCommand
pattern and is vulnerable to stdout/stderr buffering and package-manager
differences; replace the execAsync call with the existing spawn-based helper
(the same pattern used by execGitCommand/spawnProcess in this file) so you
stream and accumulate stdout/stderr into a single output string, handle timeouts
via the spawn helper, and push results.push({ check: check.name, passed:
true/false, output }) using the streamed output; additionally detect package
manager by checking for lockfiles (yarn.lock, pnpm-lock.yaml, package-lock.json)
and normalize/translate common hardcoded npm commands in verificationChecks to
the appropriate runner (yarn/pnpm/npm) before spawning so scripts work across
projects.

In `@apps/server/src/services/feature-state-manager.ts`:
- Around line 212-282: In resetStuckFeatures, add counters (e.g.,
featuresScanned and featuresReset) while iterating entries in resetStuckFeatures
and increment featuresScanned for each directory and featuresReset whenever
needsUpdate is true; after the loop (before the catch) emit a logger.info
summary like `[resetStuckFeatures] Scanned X features, reset Y features for
${projectPath}` so you get a concise post-run summary to aid debugging.
- Around line 291-340: The updateFeaturePlanSpec function persists planSpec
changes but never emits an event; after the atomicWriteJson call add a
post-persist emission using createEventEmitter() to send a "plan_spec_updated"
event (include identifying payload such as featureId and the updated
feature.planSpec or entire feature) so the frontend is notified; follow the
existing persist-before-emit pattern used by saveFeatureSummary/updateTaskStatus
and ensure you call createEventEmitter().emit(...) immediately after
atomicWriteJson completes.

In `@apps/server/src/services/pipeline-orchestrator.ts`:
- Around line 534-548: buildTestFailureSummary and extractFailedTestNames
duplicate the same line-parsing logic for detecting failures (regex for
FAIL/FAILED, leading '> ... .test|.spec.', and assertion keywords). Extract that
shared logic into a single helper function (e.g., parseTestLine or
collectFailedInfo) and replace the duplicated loops in buildTestFailureSummary
and extractFailedTestNames to call this helper; ensure the helper
returns/updates failCount, passCount, and failedTests (or returns extracted test
names and counts) and preserves the existing regexes and trimming behavior so
both callers remain in sync.

In `@apps/server/src/services/plan-approval-service.ts`:
- Around line 95-108: The timeout handler currently calls the raw reject from
the Promise constructor; move the creation of wrappedReject (and wrappedResolve
if present) to before the setTimeout call (or move the setTimeout after the
wrappers) and replace the raw reject in the timeout callback with wrappedReject
so the timeout path uses the same cleanup logic; update the timeout callback
that references setTimeout/timeoutId/pendingApprovals/featureId/timeoutMinutes
to call wrappedReject(...) and retain the delete(this.pendingApprovals) and
logger.warn behavior.

In `@apps/ui/src/components/views/graph-view-page.tsx`:
- Around line 316-328: The catch block after calling handleAddFeature and
handleStartImplementation currently only logs errors via logger.error; update it
to also show a user-facing toast by calling toast.error with a concise message
(e.g., "Failed to add and start feature") and include the error.message or
stringified error for context, so replace or augment the logger.error call in
that catch to call toast.error alongside logger.error; locate the block using
useAppStore.getState(), handleAddFeature, handleStartImplementation and
logger.error to make the change.

apps/ui/src/components/views/graph-view-page.tsx (1)

316-328: Good addition of error handling and the new-feature guard.

The snapshot-and-diff approach to detect the newly added feature is sound, and the try/catch prevents unhandled rejections. One small gap: other error paths in this file surface failures to the user via toast.error (e.g., Line 175), but this catch block only logs silently. Consider adding a toast so the user knows the "add & start" action failed.

Suggested improvement

       } catch (error) {
         logger.error('Failed to add and start feature:', error);
+        toast.error('Failed to add and start feature');
       }

🤖 Prompt for AI Agents

Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@apps/ui/src/components/views/graph-view-page.tsx` around lines 316 - 328, The
catch block after calling handleAddFeature and handleStartImplementation
currently only logs errors via logger.error; update it to also show a
user-facing toast by calling toast.error with a concise message (e.g., "Failed
to add and start feature") and include the error.message or stringified error
for context, so replace or augment the logger.error call in that catch to call
toast.error alongside logger.error; locate the block using
useAppStore.getState(), handleAddFeature, handleStartImplementation and
logger.error to make the change.

apps/server/src/services/pipeline-orchestrator.ts (1)

534-548: Duplicate FAIL-line extraction logic between buildTestFailureSummary and extractFailedTestNames.

Both methods iterate over scrollback lines with the same FAIL/FAILED regex matching. Consider extracting the shared logic into a single helper to keep them in sync.

🤖 Prompt for AI Agents

Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@apps/server/src/services/pipeline-orchestrator.ts` around lines 534 - 548,
buildTestFailureSummary and extractFailedTestNames duplicate the same
line-parsing logic for detecting failures (regex for FAIL/FAILED, leading '> ...
.test|.spec.', and assertion keywords). Extract that shared logic into a single
helper function (e.g., parseTestLine or collectFailedInfo) and replace the
duplicated loops in buildTestFailureSummary and extractFailedTestNames to call
this helper; ensure the helper returns/updates failCount, passCount, and
failedTests (or returns extracted test names and counts) and preserves the
existing regexes and trimming behavior so both callers remain in sync.

apps/server/src/services/plan-approval-service.ts (1)

95-108: Timeout callback uses raw reject instead of wrappedReject.

The timeout handler (Line 102) calls the raw reject from the Promise constructor rather than wrappedReject. While this works (the timeout doesn't need to clear itself since it already fired), it's inconsistent with the stated design of "wrapped resolve/reject ensures cleanup." If the timeout handler used wrappedReject, it would be self-documenting and safe against future refactoring that might add cleanup logic to the wrapped functions.

✨ Optional: use wrappedReject in timeout

This requires reordering the declarations so wrappedReject is defined before the setTimeout call, or moving the timeout setup after the wrappers.

+     const wrappedReject = (error: Error) => {
+       clearTimeout(timeoutId);
+       reject(error);
+     };
+
      const timeoutId = setTimeout(() => {
        const pending = this.pendingApprovals.get(key);
        if (pending) {
          logger.warn(...);
          this.pendingApprovals.delete(key);
-         reject(
+         wrappedReject(
            new Error(...)
          );
        }
      }, timeoutMs);

      const wrappedResolve = (result: PlanApprovalResult) => {
        clearTimeout(timeoutId);
        resolve(result);
      };

-     const wrappedReject = (error: Error) => {
-       clearTimeout(timeoutId);
-       reject(error);
-     };

🤖 Prompt for AI Agents

Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@apps/server/src/services/plan-approval-service.ts` around lines 95 - 108, The
timeout handler currently calls the raw reject from the Promise constructor;
move the creation of wrappedReject (and wrappedResolve if present) to before the
setTimeout call (or move the setTimeout after the wrappers) and replace the raw
reject in the timeout callback with wrappedReject so the timeout path uses the
same cleanup logic; update the timeout callback that references
setTimeout/timeoutId/pendingApprovals/featureId/timeoutMinutes to call
wrappedReject(...) and retain the delete(this.pendingApprovals) and logger.warn
behavior.

apps/server/src/services/auto-mode/facade.ts (2)

167-238: Significant code duplication: runAgentFn callback defined twice with near-identical logic.

The runAgentFn callback for PipelineOrchestrator (Lines 167-238) and ExecutionService (Lines 286-364) share nearly identical provider resolution, credential handling, and agentExecutor.execute() invocation. The same applies to buildTaskPrompt (Lines 225-235 vs 351-361). Consider extracting these into shared helper functions within the create method to avoid drift.

Also applies to: 286-364

🤖 Prompt for AI Agents

Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@apps/server/src/services/auto-mode/facade.ts` around lines 167 - 238, The
anonymous async runAgentFn bodies contain duplicated provider resolution,
credential lookup and agentExecutor.execute invocation (including
buildTaskPrompt logic); extract shared helpers inside create such as a
getResolvedProvider(model) helper that uses ProviderFactory.getProviderForModel,
stripProviderPrefix and getProviderByModelId to return {provider,
effectiveBareModel, credentials, claudeCompatibleProvider}, and extract a
buildTaskPrompt(task, allTasks, taskIndex, template, feedback) helper to
centralize the template replacements; then replace both inline blocks (the async
runAgentFn passed to PipelineOrchestrator and the one passed to
ExecutionService) to call these helpers and invoke agentExecutor.execute with
the unified parameter object and callbacks.

---

660-669: verifyFeature uses shell-based execAsync for verification commands.

While the commands are hardcoded (not user-derived), this is inconsistent with the execGitCommand pattern using spawnProcess introduced elsewhere in this file. Also, exec doesn't handle stdout/stderr buffering limits well for large outputs.

Additionally, the hardcoded npm commands may fail for projects using yarn, pnpm, or differently-named scripts.

🤖 Prompt for AI Agents

Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@apps/server/src/services/auto-mode/facade.ts` around lines 660 - 669, The
loop in verifyFeature currently uses execAsync(check.cmd) which is inconsistent
with the spawnProcess/execGitCommand pattern and is vulnerable to stdout/stderr
buffering and package-manager differences; replace the execAsync call with the
existing spawn-based helper (the same pattern used by
execGitCommand/spawnProcess in this file) so you stream and accumulate
stdout/stderr into a single output string, handle timeouts via the spawn helper,
and push results.push({ check: check.name, passed: true/false, output }) using
the streamed output; additionally detect package manager by checking for
lockfiles (yarn.lock, pnpm-lock.yaml, package-lock.json) and normalize/translate
common hardcoded npm commands in verificationChecks to the appropriate runner
(yarn/pnpm/npm) before spawning so scripts work across projects.

apps/server/src/services/feature-state-manager.ts (2)

212-282: resetStuckFeatures silently continues on per-feature read failures.

If readJsonWithRecovery returns null for a feature (Line 228), the loop skips it. This is fine. However, there's no summary log indicating how many features were reset. A brief summary log after the loop would aid debugging after server restarts.

✨ Optional: Add summary log

+      let resetCount = 0;
       for (const entry of entries) {
         ...
         if (needsUpdate) {
+          resetCount++;
           feature.updatedAt = new Date().toISOString();
           await atomicWriteJson(featurePath, feature, { backupCount: DEFAULT_BACKUP_COUNT });
         }
       }
+      if (resetCount > 0) {
+        logger.info(`[resetStuckFeatures] Reset ${resetCount} stuck feature(s) for ${projectPath}`);
+      }

🤖 Prompt for AI Agents

Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@apps/server/src/services/feature-state-manager.ts` around lines 212 - 282, In
resetStuckFeatures, add counters (e.g., featuresScanned and featuresReset) while
iterating entries in resetStuckFeatures and increment featuresScanned for each
directory and featuresReset whenever needsUpdate is true; after the loop (before
the catch) emit a logger.info summary like `[resetStuckFeatures] Scanned X
features, reset Y features for ${projectPath}` so you get a concise post-run
summary to aid debugging.

---

291-340: updateFeaturePlanSpec does not emit an event after persistence.

Unlike saveFeatureSummary and updateTaskStatus, this method persists changes but never emits an event. Callers in AgentExecutor compensate by emitting their own events (e.g. plan_approval_required, plan_approved), but if a caller forgets, the UI won't be notified of planSpec changes.

Consider emitting a plan_spec_updated event here for consistency with the persist-before-emit pattern used elsewhere in this class. As per coding guidelines, "All server operations should emit events using createEventEmitter() from lib/events.ts that stream to the frontend via WebSocket."

🤖 Prompt for AI Agents

Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@apps/server/src/services/feature-state-manager.ts` around lines 291 - 340,
The updateFeaturePlanSpec function persists planSpec changes but never emits an
event; after the atomicWriteJson call add a post-persist emission using
createEventEmitter() to send a "plan_spec_updated" event (include identifying
payload such as featureId and the updated feature.planSpec or entire feature) so
the frontend is notified; follow the existing persist-before-emit pattern used
by saveFeatureSummary/updateTaskStatus and ensure you call
createEventEmitter().emit(...) immediately after atomicWriteJson completes.

🤖 Fix all nitpicks with AI agents

Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.

In `@apps/server/src/services/auto-loop-coordinator.ts`:
- Around line 46-51: The local ExecuteFeatureFn type in auto-loop-coordinator.ts
diverges from the canonical signature; replace the local declaration with an
import of ExecuteFeatureFn from execution-types.ts (or update it to include the
missing parameters providedWorktreePath and options matching the canonical
definition) so the function signature (ExecuteFeatureFn) aligns with the
exported type and avoids drift.
- Around line 68-82: The AutoLoopCoordinator constructor currently accepts 10
positional dependencies (constructor in class AutoLoopCoordinator), which is
hard to read and extend; refactor it to accept a single options/config object
(e.g., AutoLoopCoordinatorOptions) containing named properties for eventBus,
concurrencyManager, settingsService, executeFeatureFn, loadPendingFeaturesFn,
saveExecutionStateFn, clearExecutionStateFn, resetStuckFeaturesFn,
isFeatureFinishedFn and isFeatureRunningFn, update the constructor to
destructure and assign those properties to the private fields, and update all
instantiation sites to pass a single options object (preserving original types
and nullability for settingsService) so call-sites remain readable and future
optional deps can be added without changing call signatures.
- Around line 184-202: The loop currently picks only one nextFeature per
iteration causing slow ramp-up; change the logic in the auto-loop to compute
available slots as (projectState.config.maxConcurrency || maxConcurrency) minus
the current runningCount, then select and launch up to that many features from
pendingFeatures that satisfy !this.isFeatureRunningFn(f.id) &&
!this.isFeatureFinishedFn(f). For each selected feature call
this.executeFeatureFn(projectPath, feature.id, projectState.config.useWorktrees,
true) and attach the same .catch handler that classifies the error
(classifyError), logs via logger.error, and calls
this.trackFailureAndCheckPauseForProject and this.signalShouldPauseForProject as
now; keep the final await this.sleep(2000, projectState.abortController.signal)
unchanged. Ensure runningCount/slot computation uses the same source of truth
used elsewhere (e.g., projectState or config) and that launches are started
without awaiting so they run concurrently.
- Around line 115-120: The config object in startAutoLoopForProject hardcodes
useWorktrees to true; change this by adding an optional useWorktrees?: boolean
parameter to startAutoLoopForProject (and propagate it from callers), update the
AutoModeConfig definition to make useWorktrees optional (or keep boolean), and
set useWorktrees: useWorktrees ?? true when building the config instead of the
literal true so existing behavior is preserved while allowing callers to toggle
it; ensure all call sites pass the new flag or rely on the default.
- Around line 203-206: The bare catch swallows all errors; change it to catch
(err) and, after checking projectState.abortController.signal.aborted, log a
warning containing the error and contextual info (e.g., which worktree/project
and the functions involved like
loadPendingFeaturesFn/getRunningCountForWorktree) using the existing logger
(e.g., this.logger.warn or projectState.logger) or console.warn, then await
this.sleep as before; ensure you do not log when the abort signal caused the
break.

apps/server/src/services/auto-loop-coordinator.ts (5)

46-51: ExecuteFeatureFn type diverges from the canonical definition in execution-types.ts.

The local type declares 4 parameters, but execution-types.ts defines ExecuteFeatureFn with 6 parameters (including optional providedWorktreePath and options). Maintaining two separate type definitions for the same concept creates a drift risk — if the canonical signature changes, this local copy won't reflect it.

Consider importing the type from execution-types.ts instead of re-declaring it, or at minimum keeping the signatures aligned.

#!/bin/bash
# Check if ExecuteFeatureFn is exported from execution-types and used elsewhere
rg -n 'ExecuteFeatureFn' --type=ts -C2

🤖 Prompt for AI Agents

Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@apps/server/src/services/auto-loop-coordinator.ts` around lines 46 - 51, The
local ExecuteFeatureFn type in auto-loop-coordinator.ts diverges from the
canonical signature; replace the local declaration with an import of
ExecuteFeatureFn from execution-types.ts (or update it to include the missing
parameters providedWorktreePath and options matching the canonical definition)
so the function signature (ExecuteFeatureFn) aligns with the exported type and
avoids drift.

---

68-82: Constructor takes 10 dependencies — consider a config/options object.

The constructor has 10 positional parameters, making call-site readability and future extensibility difficult. An options object pattern would be more maintainable.

🤖 Prompt for AI Agents

Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@apps/server/src/services/auto-loop-coordinator.ts` around lines 68 - 82, The
AutoLoopCoordinator constructor currently accepts 10 positional dependencies
(constructor in class AutoLoopCoordinator), which is hard to read and extend;
refactor it to accept a single options/config object (e.g.,
AutoLoopCoordinatorOptions) containing named properties for eventBus,
concurrencyManager, settingsService, executeFeatureFn, loadPendingFeaturesFn,
saveExecutionStateFn, clearExecutionStateFn, resetStuckFeaturesFn,
isFeatureFinishedFn and isFeatureRunningFn, update the constructor to
destructure and assign those properties to the private fields, and update all
instantiation sites to pass a single options object (preserving original types
and nullability for settingsService) so call-sites remain readable and future
optional deps can be added without changing call signatures.

---

184-202: Loop launches at most one feature per iteration — slow ramp-up at higher concurrency.

With maxConcurrency set to, say, 5 and 0 features running, the loop picks only one feature per 2-second cycle (line 202). It would take ~10 seconds to fill all slots. Consider launching up to maxConcurrency - runningCount features per iteration for faster utilization.

♻️ Sketch of batched launch

-       const nextFeature = pendingFeatures.find(
-         (f) => !this.isFeatureRunningFn(f.id) && !this.isFeatureFinishedFn(f)
-       );
-       if (nextFeature) {
-         projectState.hasEmittedIdleEvent = false;
-         this.executeFeatureFn(
-           projectPath,
-           nextFeature.id,
-           projectState.config.useWorktrees,
-           true
-         ).catch((error) => {
-           const errorInfo = classifyError(error);
-           logger.error(`Auto-loop feature ${nextFeature.id} failed:`, errorInfo.message);
-           if (this.trackFailureAndCheckPauseForProject(projectPath, branchName, errorInfo)) {
-             this.signalShouldPauseForProject(projectPath, branchName, errorInfo);
-           }
-         });
-       }
+       const availableSlots = projectState.config.maxConcurrency - runningCount;
+       const eligibleFeatures = pendingFeatures.filter(
+         (f) => !this.isFeatureRunningFn(f.id) && !this.isFeatureFinishedFn(f)
+       );
+       const toLaunch = eligibleFeatures.slice(0, availableSlots);
+       if (toLaunch.length > 0) {
+         projectState.hasEmittedIdleEvent = false;
+         for (const feature of toLaunch) {
+           this.executeFeatureFn(
+             projectPath,
+             feature.id,
+             projectState.config.useWorktrees,
+             true
+           ).catch((error) => {
+             const errorInfo = classifyError(error);
+             logger.error(`Auto-loop feature ${feature.id} failed:`, errorInfo.message);
+             if (this.trackFailureAndCheckPauseForProject(projectPath, branchName, errorInfo)) {
+               this.signalShouldPauseForProject(projectPath, branchName, errorInfo);
+             }
+           });
+         }
+       }

🤖 Prompt for AI Agents

Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@apps/server/src/services/auto-loop-coordinator.ts` around lines 184 - 202,
The loop currently picks only one nextFeature per iteration causing slow
ramp-up; change the logic in the auto-loop to compute available slots as
(projectState.config.maxConcurrency || maxConcurrency) minus the current
runningCount, then select and launch up to that many features from
pendingFeatures that satisfy !this.isFeatureRunningFn(f.id) &&
!this.isFeatureFinishedFn(f). For each selected feature call
this.executeFeatureFn(projectPath, feature.id, projectState.config.useWorktrees,
true) and attach the same .catch handler that classifies the error
(classifyError), logs via logger.error, and calls
this.trackFailureAndCheckPauseForProject and this.signalShouldPauseForProject as
now; keep the final await this.sleep(2000, projectState.abortController.signal)
unchanged. Ensure runningCount/slot computation uses the same source of truth
used elsewhere (e.g., projectState or config) and that launches are started
without awaiting so they run concurrently.

---

115-120: useWorktrees is hardcoded to true — consider making it configurable or removing from config.

The AutoModeConfig interface declares useWorktrees as a field, but startAutoLoopForProject always sets it to true (line 117). If worktree usage isn't meant to be toggled, remove it from the interface to avoid misleading consumers; otherwise, accept it as a parameter.

🤖 Prompt for AI Agents

Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@apps/server/src/services/auto-loop-coordinator.ts` around lines 115 - 120,
The config object in startAutoLoopForProject hardcodes useWorktrees to true;
change this by adding an optional useWorktrees?: boolean parameter to
startAutoLoopForProject (and propagate it from callers), update the
AutoModeConfig definition to make useWorktrees optional (or keep boolean), and
set useWorktrees: useWorktrees ?? true when building the config instead of the
literal true so existing behavior is preserved while allowing callers to toggle
it; ensure all call sites pass the new flag or rely on the default.

---

203-206: Bare catch silently swallows all loop errors.

Any unexpected error from loadPendingFeaturesFn or getRunningCountForWorktree is caught at line 203 with no logging. Only the abort-signal check prevents a silent infinite retry. Consider logging a warning for non-abort errors to aid debugging.

♻️ Proposed fix

-     } catch {
+     } catch (error) {
        if (projectState.abortController.signal.aborted) break;
+       logger.warn(`Auto-loop iteration error for ${worktreeKey}:`, error);
        await this.sleep(5000, projectState.abortController.signal);
      }

🤖 Prompt for AI Agents

Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@apps/server/src/services/auto-loop-coordinator.ts` around lines 203 - 206,
The bare catch swallows all errors; change it to catch (err) and, after checking
projectState.abortController.signal.aborted, log a warning containing the error
and contextual info (e.g., which worktree/project and the functions involved
like loadPendingFeaturesFn/getRunningCountForWorktree) using the existing logger
(e.g., this.logger.warn or projectState.logger) or console.warn, then await
this.sleep as before; ensure you do not log when the abort signal caused the
break.

🤖 Fix all nitpicks with AI agents

Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.

In `@apps/server/src/services/agent-executor.ts`:
- Around line 126-144: The appendRawEvent helper is writing pretty-printed
multi-line JSON (JSON.stringify with spacing) which breaks JSONL; change it to
emit a single-line JSON per event by removing the space/indent argument so each
push to rawOutputLines is JSON.stringify({ timestamp: ..., event }) (ensure
rawOutputLines, rawOutputPath, appendRawEvent and the debounce logic using
AgentExecutor.WRITE_DEBOUNCE_MS remain unchanged), and retain the existing
write/debounce/try-catch behavior so the file appends one JSON object per line
compatible with JSONL.
- Around line 554-555: In provider.executeQuery(...this.buildExecOpts(...,
sdkOptions?.maxTurns || 100)) replace the fallback operator with nullish
coalescing to match the earlier use; change the expression using
sdkOptions?.maxTurns || 100 to sdkOptions?.maxTurns ?? 100 so an explicit 0 is
honored (look for provider.executeQuery and buildExecOpts references and align
with the sdkOptions?.maxTurns ?? 50 pattern used elsewhere).

In `@apps/server/src/services/auto-mode/facade.ts`:
- Around line 668-677: The verifyFeature loop currently calls
execAsync(check.cmd) which spawns a shell; replace this with a non-shell
invocation for consistency by using execFile or the existing
execGitCommand/spawnProcess helpers: split the hardcoded command string in each
verificationChecks entry into command + args (or update entries to provide cmd
and args), call execFile(cmd, args, { cwd: workDir, timeout: 120000 }) or call
execGitCommand/spawnProcess equivalent to run the command without a shell, then
keep the same success/error handling that pushes to results and flips allPassed
on catch. Ensure you reference the verificationChecks items, the verifyFeature
loop, and preserve output capture and the existing error.message usage.
- Around line 98-433: The create factory method contains duplicated runAgentFn
logic used by PipelineOrchestrator and ExecutionService (see
PipelineOrchestrator constructor call and ExecutionService constructor call)
which should be extracted into a single helper to avoid divergence; implement a
private/shared function (e.g., buildAgentRunner or createRunAgentFn) that
captures dependencies (agentExecutor, settingsService, getProviderByModelId,
stripProviderPrefix, ProviderFactory, planApprovalService, featureStateManager)
and returns the async runAgentFn signature expected by both PipelineOrchestrator
and ExecutionService, then replace the inline implementations in the
PipelineOrchestrator and ExecutionService calls with calls to that helper to
eliminate duplication and keep behavior consistent.
- Line 148: The non-null assertions on facadeInstance used inside callbacks
passed to PipelineOrchestrator, AutoLoopCoordinator, and ExecutionService are
fragile because facadeInstance is only assigned later; add a clear comment above
the declaration of facadeInstance describing the invariant ("callbacks passed to
PipelineOrchestrator / AutoLoopCoordinator / ExecutionService must not be
invoked synchronously and may reference facadeInstance after it's assigned") so
future maintainers know why facadeInstance! is safe, and optionally replace
direct facadeInstance! usage with a small local accessor function (e.g.,
getFacade() that throws a descriptive error if null) to make failures explicit;
reference the variable facadeInstance and the constructors PipelineOrchestrator,
AutoLoopCoordinator, ExecutionService when making the change.

In `@apps/server/src/services/execution-service.ts`:
- Around line 63-84: The ExecutionService constructor currently takes 18
positional params (notably many callback fns like runAgentFn, executePipelineFn,
updateFeatureStatusFn, loadFeatureFn, etc.), which is fragile; refactor by
introducing an ExecutionServiceCallbacks interface that groups all callback
function types (e.g., runAgent, executePipeline, updateFeatureStatus,
loadFeature, getPlanningPromptPrefix, saveFeatureSummary, recordLearnings,
contextExists, resumeFeature, trackFailure, signalPause, recordSuccess,
saveExecutionState, loadContextFiles), then change the ExecutionService
constructor signature to accept the four core deps (eventBus,
concurrencyManager, worktreeResolver, settingsService) plus a single callbacks
object; update usages/wiring (e.g., in facade.ts) to pass a callbacks object
rather than positional args and update internal references from runAgentFn ->
callbacks.runAgent, etc.

In `@apps/server/src/services/feature-state-manager.ts`:
- Around line 330-339: The version bump logic incorrectly uses a falsy check (if
(updates.content && ...)) so setting updates.content to an empty string won't
trigger a version increment; change the condition in the update block that
currently reads against updates.content to explicitly check for presence (e.g.,
updates.content !== undefined) and compare it to oldContent before incrementing
feature.planSpec.version on feature.planSpec (ensure you reference the
oldContent captured from feature.planSpec.content and update
feature.planSpec.version accordingly).
- Around line 432-449: The code silently ignores cases where taskId doesn't
match any entry in feature.planSpec.tasks; add an else branch after the task
lookup in the method that logs a warning (including featureId, projectPath,
taskId and maybe available task IDs) so mismatches are visible; do this near the
block that updates task.status, feature.updatedAt, calls atomicWriteJson and
emitAutoModeEvent (referencing the same function scope where task is found) to
ensure a warning is emitted when task === undefined.

In `@apps/server/src/services/pipeline-orchestrator.ts`:
- Around line 554-587: The parser in parseTestLines currently treats any line
containing "toBe" or "toEqual" (and the generic "AssertionError") as a failed
test which causes false positives; update the heuristic to only record such
lines as failures when they appear in a failure context (e.g., line begins with
"AssertionError" or contains "Error" or the word "FAIL"/"FAILED" nearby) or when
associated with a test file marker (the current /^>\s+.*\.(test|spec)\./ check),
and consolidate failure-detection logic so failedTests, failCount and passCount
are incremented consistently; specifically tighten matching for "toBe"/"toEqual"
and "AssertionError" (in parseTestLines) to require contextual cues rather than
matching any occurrence.
- Around line 565-584: The current loose heuristic pushes any line with
"toBe"/"toEqual" into failedTests; tighten it by replacing the trimmed.includes
checks with a stricter pattern match: only add trimmed to failedTests if it
matches assertion-call patterns like
/\.to(Be|Equal)\s*\(|/expect\(.+\)\.(toBe|toEqual)\s*\(/ or if the line contains
explicit failure context such as "AssertionError" or "Expected"/"Received" or
the immediately preceding line indicated a FAIL/FAILED; update the code inside
the for (const line of lines) loop (variables: trimmed, failedTests,
passCount/failCount) to use these regex checks instead of the current substring
checks so passing/comments containing "toBe"/"toEqual" are not misclassified.

apps/server/src/services/feature-state-manager.ts (2)

330-339: Falsy check on updates.content silently skips version bump for empty-string content.

Line 337: if (updates.content && ...) — if someone sets content to "", the version won't increment even though content changed. Consider using updates.content !== undefined if the intent is to detect any deliberate content change.

This is a minor edge case — plan content is unlikely to be intentionally empty.

Stricter check (if needed)

-      if (updates.content && updates.content !== oldContent) {
+      if (updates.content !== undefined && updates.content !== oldContent) {

🤖 Prompt for AI Agents

Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@apps/server/src/services/feature-state-manager.ts` around lines 330 - 339,
The version bump logic incorrectly uses a falsy check (if (updates.content &&
...)) so setting updates.content to an empty string won't trigger a version
increment; change the condition in the update block that currently reads against
updates.content to explicitly check for presence (e.g., updates.content !==
undefined) and compare it to oldContent before incrementing
feature.planSpec.version on feature.planSpec (ensure you reference the
oldContent captured from feature.planSpec.content and update
feature.planSpec.version accordingly).

---

432-449: Missing warning when task is not found by ID.

If the taskId doesn't match any task in planSpec.tasks, the method silently returns without logging. This makes debugging subtle task-ID mismatches difficult. Consider adding an else branch:

Add a warning for missing task

       const task = feature.planSpec.tasks.find((t) => t.id === taskId);
       if (task) {
         task.status = status;
         feature.updatedAt = new Date().toISOString();
 
         // PERSIST BEFORE EMIT
         await atomicWriteJson(featurePath, feature, { backupCount: DEFAULT_BACKUP_COUNT });
 
         // Emit event for UI update
         this.emitAutoModeEvent('auto_mode_task_status', {
           featureId,
           projectPath,
           taskId,
           status,
           tasks: feature.planSpec.tasks,
         });
+      } else {
+        logger.warn(`Task ${taskId} not found in feature ${featureId}`);
       }

🤖 Prompt for AI Agents

Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@apps/server/src/services/feature-state-manager.ts` around lines 432 - 449,
The code silently ignores cases where taskId doesn't match any entry in
feature.planSpec.tasks; add an else branch after the task lookup in the method
that logs a warning (including featureId, projectPath, taskId and maybe
available task IDs) so mismatches are visible; do this near the block that
updates task.status, feature.updatedAt, calls atomicWriteJson and
emitAutoModeEvent (referencing the same function scope where task is found) to
ensure a warning is emitted when task === undefined.

apps/server/src/services/pipeline-orchestrator.ts (2)

554-587: Consider the typo 'AssertionError' — should this be 'AssertionError'?

Hmm, on closer inspection both spellings resolve to the same string. However, note that this heuristic parser is fairly fragile — it matches any line containing toBe/toEqual as a "failed test" even if it appears in passing test output or comments. This is a minor concern given the context (building a summary for agent consumption), so not blocking.

🤖 Prompt for AI Agents

Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@apps/server/src/services/pipeline-orchestrator.ts` around lines 554 - 587,
The parser in parseTestLines currently treats any line containing "toBe" or
"toEqual" (and the generic "AssertionError") as a failed test which causes false
positives; update the heuristic to only record such lines as failures when they
appear in a failure context (e.g., line begins with "AssertionError" or contains
"Error" or the word "FAIL"/"FAILED" nearby) or when associated with a test file
marker (the current /^>\s+.*\.(test|spec)\./ check), and consolidate
failure-detection logic so failedTests, failCount and passCount are incremented
consistently; specifically tighten matching for "toBe"/"toEqual" and
"AssertionError" (in parseTestLines) to require contextual cues rather than
matching any occurrence.

---

565-584: Heuristic test parsing is loose — lines matching toBe/toEqual anywhere get added as "failed tests".

Lines 579-580 match any line containing toBe or toEqual, which could appear in passing assertions or comments, inflating the failure list. Since this feeds an agent prompt (not user-facing logic), it's a minor concern — the deduplication and cap at 10/20 entries mitigate the noise.

🤖 Prompt for AI Agents

Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@apps/server/src/services/pipeline-orchestrator.ts` around lines 565 - 584,
The current loose heuristic pushes any line with "toBe"/"toEqual" into
failedTests; tighten it by replacing the trimmed.includes checks with a stricter
pattern match: only add trimmed to failedTests if it matches assertion-call
patterns like /\.to(Be|Equal)\s*\(|/expect\(.+\)\.(toBe|toEqual)\s*\(/ or if the
line contains explicit failure context such as "AssertionError" or
"Expected"/"Received" or the immediately preceding line indicated a FAIL/FAILED;
update the code inside the for (const line of lines) loop (variables: trimmed,
failedTests, passCount/failCount) to use these regex checks instead of the
current substring checks so passing/comments containing "toBe"/"toEqual" are not
misclassified.

apps/server/src/services/agent-executor.ts (2)

126-144: Raw JSONL output uses pretty-printed JSON — not valid JSONL.

Line 130 serializes with JSON.stringify(..., null, 4), producing multi-line entries. JSONL requires one JSON object per line. Since this is a debug-only feature gated by AUTOMAKER_DEBUG_RAW_OUTPUT, it's low impact, but tools expecting JSONL will choke on the output.

Suggested fix

-          JSON.stringify({ timestamp: new Date().toISOString(), event }, null, 4)
+          JSON.stringify({ timestamp: new Date().toISOString(), event })

🤖 Prompt for AI Agents

Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@apps/server/src/services/agent-executor.ts` around lines 126 - 144, The
appendRawEvent helper is writing pretty-printed multi-line JSON (JSON.stringify
with spacing) which breaks JSONL; change it to emit a single-line JSON per event
by removing the space/indent argument so each push to rawOutputLines is
JSON.stringify({ timestamp: ..., event }) (ensure rawOutputLines, rawOutputPath,
appendRawEvent and the debounce logic using AgentExecutor.WRITE_DEBOUNCE_MS
remain unchanged), and retain the existing write/debounce/try-catch behavior so
the file appends one JSON object per line compatible with JSONL.

---

554-555: Inconsistent nullish handling: || here vs ?? on line 336.

Line 336 uses sdkOptions?.maxTurns ?? 50 (null-coalescing), but line 555 uses sdkOptions?.maxTurns || 100 (falsy-coalescing). If maxTurns were explicitly 0, || would silently substitute 100. Consider using ?? for consistency.

-            this.buildExecOpts(options, revPrompt, sdkOptions?.maxTurns || 100)
+            this.buildExecOpts(options, revPrompt, sdkOptions?.maxTurns ?? 100)

🤖 Prompt for AI Agents

Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@apps/server/src/services/agent-executor.ts` around lines 554 - 555, In
provider.executeQuery(...this.buildExecOpts(..., sdkOptions?.maxTurns || 100))
replace the fallback operator with nullish coalescing to match the earlier use;
change the expression using sdkOptions?.maxTurns || 100 to sdkOptions?.maxTurns
?? 100 so an explicit 0 is honored (look for provider.executeQuery and
buildExecOpts references and align with the sdkOptions?.maxTurns ?? 50 pattern
used elsewhere).

apps/server/src/services/auto-mode/facade.ts (3)

668-677: verifyFeature uses execAsync (shell) for hardcoded commands — consider execFile for consistency.

The rest of the file migrated git operations to execGitCommand (which uses spawnProcess/array args). verifyFeature still uses execAsync which spawns a shell. Since the commands are hardcoded string literals, there's no injection risk here, but switching to execFile (or splitting command/args) would be more consistent with the rest of the file.

🤖 Prompt for AI Agents

Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@apps/server/src/services/auto-mode/facade.ts` around lines 668 - 677, The
verifyFeature loop currently calls execAsync(check.cmd) which spawns a shell;
replace this with a non-shell invocation for consistency by using execFile or
the existing execGitCommand/spawnProcess helpers: split the hardcoded command
string in each verificationChecks entry into command + args (or update entries
to provide cmd and args), call execFile(cmd, args, { cwd: workDir, timeout:
120000 }) or call execGitCommand/spawnProcess equivalent to run the command
without a shell, then keep the same success/error handling that pushes to
results and flips allPassed on catch. Ensure you reference the
verificationChecks items, the verifyFeature loop, and preserve output capture
and the existing error.message usage.

---

98-433: The create factory method is ~340 lines of complex wiring — consider extracting.

This static factory wires 7+ services with interleaved callback definitions and duplicated runAgentFn implementations (lines 167–238 and lines 286–363 are near-identical). Extracting the runAgentFn builder into a shared helper would reduce duplication and the risk of them diverging.

🤖 Prompt for AI Agents

Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@apps/server/src/services/auto-mode/facade.ts` around lines 98 - 433, The
create factory method contains duplicated runAgentFn logic used by
PipelineOrchestrator and ExecutionService (see PipelineOrchestrator constructor
call and ExecutionService constructor call) which should be extracted into a
single helper to avoid divergence; implement a private/shared function (e.g.,
buildAgentRunner or createRunAgentFn) that captures dependencies (agentExecutor,
settingsService, getProviderByModelId, stripProviderPrefix, ProviderFactory,
planApprovalService, featureStateManager) and returns the async runAgentFn
signature expected by both PipelineOrchestrator and ExecutionService, then
replace the inline implementations in the PipelineOrchestrator and
ExecutionService calls with calls to that helper to eliminate duplication and
keep behavior consistent.

---

148-148: facadeInstance! non-null assertions rely on deferred callback invocation.

The callbacks passed to PipelineOrchestrator, AutoLoopCoordinator, and ExecutionService (lines 165, 251, 269, etc.) use facadeInstance! which is only assigned at line 416. This is safe only if none of these constructors invoke the callbacks synchronously. Worth a brief comment to document this invariant for future maintainers.

🤖 Prompt for AI Agents

Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@apps/server/src/services/auto-mode/facade.ts` at line 148, The non-null
assertions on facadeInstance used inside callbacks passed to
PipelineOrchestrator, AutoLoopCoordinator, and ExecutionService are fragile
because facadeInstance is only assigned later; add a clear comment above the
declaration of facadeInstance describing the invariant ("callbacks passed to
PipelineOrchestrator / AutoLoopCoordinator / ExecutionService must not be
invoked synchronously and may reference facadeInstance after it's assigned") so
future maintainers know why facadeInstance! is safe, and optionally replace
direct facadeInstance! usage with a small local accessor function (e.g.,
getFacade() that throws a descriptive error if null) to make failures explicit;
reference the variable facadeInstance and the constructors PipelineOrchestrator,
AutoLoopCoordinator, ExecutionService when making the change.

apps/server/src/services/execution-service.ts (1)

63-84: Constructor accepts 18 positional parameters — fragile and hard to maintain.

With 14 callback parameters all typed as function signatures, it's easy to swap arguments during wiring (as seen in facade.ts which has to carefully order them). Consider grouping the callbacks into an options/config object.

Sketch

interface ExecutionServiceCallbacks {
  runAgent: RunAgentFn;
  executePipeline: ExecutePipelineFn;
  updateFeatureStatus: UpdateFeatureStatusFn;
  loadFeature: LoadFeatureFn;
  // ...etc
}

constructor(
  private eventBus: TypedEventBus,
  private concurrencyManager: ConcurrencyManager,
  private worktreeResolver: WorktreeResolver,
  private settingsService: SettingsService | null,
  private callbacks: ExecutionServiceCallbacks
) {}

🤖 Prompt for AI Agents

Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@apps/server/src/services/execution-service.ts` around lines 63 - 84, The
ExecutionService constructor currently takes 18 positional params (notably many
callback fns like runAgentFn, executePipelineFn, updateFeatureStatusFn,
loadFeatureFn, etc.), which is fragile; refactor by introducing an
ExecutionServiceCallbacks interface that groups all callback function types
(e.g., runAgent, executePipeline, updateFeatureStatus, loadFeature,
getPlanningPromptPrefix, saveFeatureSummary, recordLearnings, contextExists,
resumeFeature, trackFailure, signalPause, recordSuccess, saveExecutionState,
loadContextFiles), then change the ExecutionService constructor signature to
accept the four core deps (eventBus, concurrencyManager, worktreeResolver,
settingsService) plus a single callbacks object; update usages/wiring (e.g., in
facade.ts) to pass a callbacks object rather than positional args and update
internal references from runAgentFn -> callbacks.runAgent, etc.

🤖 Fix all nitpicks with AI agents

Verify each finding against the current code and only fix it if needed.


In `@apps/server/src/services/agent-executor.ts`:
- Around line 518-520: The variables hasFeedback and hasEdits are currently set
to `.length` (a number or undefined) which relies on truthiness; change them to
explicit booleans to match their names — e.g., set hasFeedback =
Boolean(approvalResult.feedback?.trim().length) or hasFeedback =
(approvalResult.feedback?.trim().length ?? 0) > 0 and likewise for hasEdits —
then keep the existing conditional that throws the Error('Plan cancelled by
user') when both are false; update the expressions near approvalResult.feedback
and approvalResult.editedPlan to use the chosen explicit boolean form.
- Around line 52-276: The execute method is too large and mixes concerns;
extract the file-write setup and the stream-processing loop into private helper
methods to reduce cyclomatic complexity. Move the writeToFile, scheduleWrite,
and appendRawEvent logic into a private method (e.g., setupFileWriters or
flushHelpers) that returns the helper functions and cleanup handles; extract the
for-await-of provider.executeQuery loop body (the streaming logic that handles
msg types, text/tool blocks, spec detection and calls to
handleSpecGenerated/executeTasksLoop) into a private method (e.g.,
processStreamMessages) that accepts the stream iterator, options, callbacks, and
mutable state (responseText, specDetected, tasksCompleted, aborted) and returns
the updated state; keep execute as the orchestration layer that calls
setupFileWriters, then processStreamMessages, handles cleanup (clearing timers,
final writes) and returns the result.

In `@apps/server/src/services/auto-mode/facade.ts`:
- Around line 625-634: The loop in verifyFeature uses shell-based execAsync for
running verification commands (see the loop referencing execAsync and
verificationChecks), which risks shell metacharacter interpretation of workDir;
replace execAsync with the non-shell spawnProcess (or the same helper used by
execGitCommand) to run commands without a shell, passing args and cwd
explicitly, wiring stdout/stderr streams to capture output (concatenate or
prefer stdout then stderr), enforce the same timeout behavior, and preserve the
existing result handling (push {check: check.name, passed: true/false, output:
...} and set allPassed = false on error and break). Ensure you invoke
spawnProcess with the command split into program + args (from check.cmd), handle
process exit vs thrown errors to populate (error).message or collected output,
and keep the break behavior identical.
- Around line 560-587: The stubbed followUpFeature currently calls
this.concurrencyManager.acquire (stored in runningEntry) and then always throws,
causing an unnecessary acquire/release cycle; remove the acquire and the
corresponding this.concurrencyManager.release call (and the runningEntry
variable) from the facade implementation so the method simply throws and
performs the existing error classification/event emission without reserving a
concurrency slot; if you want to keep a precondition check instead, explicitly
document and perform a non-mutating check (not acquire) using concurrencyManager
APIs, but do not call acquire/release in the stub.
- Around line 655-709: commitFeature currently calls
this.featureStateManager.loadFeature twice; hoist the feature load into a single
variable used throughout. Specifically, in commitFeature load the feature once
(e.g., const feature = await
this.featureStateManager.loadFeature(this.projectPath, featureId)) before the
worktree resolution logic, then use that feature for branchName lookup with
worktreeResolver.findWorktreeForBranch(...) and later for building the commit
message and event payload; remove the later redundant load so the function only
performs one feature load.
- Around line 14-48: The file imports exec and creates execAsync but those are
only used by verifyFeature; if you refactor verifyFeature to use spawnProcess
(or otherwise remove shell-based verification), delete the exec import and the
execAsync constant to avoid dead code; search for verifyFeature and replace its
use of execAsync with spawnProcess (or remove the verification block) and then
remove the now-unused exec/execAsync references from the top of facade.ts.
- Around line 146-159: Add a unit test to guard the deferred facade binding:
instantiate the module that calls create() (the factory that assigns
facadeInstance) and assert that after calling create() the closure-backed
accessor getFacade() returns a non-null AutoModeServiceFacade (and does not
throw). Target the public factory/function that performs the creation (the
create() call that sets facadeInstance) and exercise the callbacks passed to
PipelineOrchestrator/AutoLoopCoordinator/ExecutionService asynchronously to
ensure the invariant holds; fail the test if getFacade() throws or returns null.

In `@apps/server/src/services/pipeline-orchestrator.ts`:
- Around line 56-70: The PipelineOrchestrator constructor currently accepts 12
positional dependencies (TypedEventBus, FeatureStateManager, AgentExecutor,
TestRunnerService, WorktreeResolver, ConcurrencyManager, SettingsService|null,
UpdateFeatureStatusFn, loadContextFiles, BuildFeaturePromptFn, ExecuteFeatureFn,
RunAgentFn), which is unwieldy; refactor the constructor to accept a single
options/deps object (e.g. type PipelineOrchestratorOptions) that contains those
named properties, update the class to destructure that options object in the
constructor, and then update all call sites and tests that instantiate
PipelineOrchestrator to pass a named object instead of a long positional
argument list, preserving existing types and nullability for SettingsService and
keeping the same property names as in the original constructor.

apps/server/src/services/auto-mode/facade.ts (5)

625-634: verifyFeature still uses shell-based execAsync for running verification commands.

While the commands themselves are hardcoded (npm run lint, etc.), execAsync spawns a shell, which means the cwd path is interpreted by the shell. Consider using spawnProcess (or a similar non-shell API) consistent with how execGitCommand works, to avoid any shell metacharacter interpretation of workDir.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/services/auto-mode/facade.ts` around lines 625 - 634, The
loop in verifyFeature uses shell-based execAsync for running verification
commands (see the loop referencing execAsync and verificationChecks), which
risks shell metacharacter interpretation of workDir; replace execAsync with the
non-shell spawnProcess (or the same helper used by execGitCommand) to run
commands without a shell, passing args and cwd explicitly, wiring stdout/stderr
streams to capture output (concatenate or prefer stdout then stderr), enforce
the same timeout behavior, and preserve the existing result handling (push
{check: check.name, passed: true/false, output: ...} and set allPassed = false
on error and break). Ensure you invoke spawnProcess with the command split into
program + args (from check.cmd), handle process exit vs thrown errors to
populate (error).message or collected output, and keep the break behavior
identical.

---

560-587: followUpFeature acquires a concurrency slot only to immediately release it.

The stub acquires a slot (line 560), then throws (line 569), which triggers the finally block to release (line 586). While this is safe, the acquire/release is wasted work. Since this is a known stub, the concurrency slot acquisition could be removed until the method is implemented — unless you specifically want to validate that the concurrency slot can be acquired as a precondition check.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/services/auto-mode/facade.ts` around lines 560 - 587, The
stubbed followUpFeature currently calls this.concurrencyManager.acquire (stored
in runningEntry) and then always throws, causing an unnecessary acquire/release
cycle; remove the acquire and the corresponding this.concurrencyManager.release
call (and the runningEntry variable) from the facade implementation so the
method simply throws and performs the existing error classification/event
emission without reserving a concurrency slot; if you want to keep a
precondition check instead, explicitly document and perform a non-mutating check
(not acquire) using concurrencyManager APIs, but do not call acquire/release in
the stub.

---

655-709: Feature is loaded twice in commitFeature — once for worktree resolution, once for commit message.

When providedWorktreePath is falsy, loadFeature is called at line 667, and then unconditionally again at line 686. Hoist the load to avoid the redundant I/O:

♻️ Proposed fix

   async commitFeature(featureId: string, providedWorktreePath?: string): Promise<string | null> {
     let workDir = this.projectPath;
+    const feature = await this.featureStateManager.loadFeature(this.projectPath, featureId);
 
     if (providedWorktreePath) {
       try {
         await secureFs.access(providedWorktreePath);
         workDir = providedWorktreePath;
       } catch {
         // Use project path
       }
     } else {
-      const feature = await this.featureStateManager.loadFeature(this.projectPath, featureId);
       const branchName = feature?.branchName;
       if (branchName) {
         ...
       }
     }
 
     try {
       ...
-      const feature = await this.featureStateManager.loadFeature(this.projectPath, featureId);
       const title =
         feature?.description?.split('\n')[0]?.substring(0, 60) || `Feature ${featureId}`;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/services/auto-mode/facade.ts` around lines 655 - 709,
commitFeature currently calls this.featureStateManager.loadFeature twice; hoist
the feature load into a single variable used throughout. Specifically, in
commitFeature load the feature once (e.g., const feature = await
this.featureStateManager.loadFeature(this.projectPath, featureId)) before the
worktree resolution logic, then use that feature for branchName lookup with
worktreeResolver.findWorktreeForBranch(...) and later for building the commit
message and event payload; remove the later redundant load so the function only
performs one feature load.

---

14-48: exec import and execAsync are only used by verifyFeature — consider removing if verification is refactored.

exec (line 15) and execAsync (line 48) exist solely for the shell-based verification commands. If verifyFeature is updated to use spawnProcess, these become dead imports. Minor cleanup to track.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/services/auto-mode/facade.ts` around lines 14 - 48, The file
imports exec and creates execAsync but those are only used by verifyFeature; if
you refactor verifyFeature to use spawnProcess (or otherwise remove shell-based
verification), delete the exec import and the execAsync constant to avoid dead
code; search for verifyFeature and replace its use of execAsync with
spawnProcess (or remove the verification block) and then remove the now-unused
exec/execAsync references from the top of facade.ts.

---

146-159: Deferred facade binding pattern is sound but fragile.

The getFacade() closure with a null-check guard is a reasonable approach. The comment documenting the invariant (callbacks are only invoked asynchronously, never during construction) is appreciated. However, if any downstream service changes to invoke a callback synchronously during construction, this will throw a cryptic error.

Consider adding a brief unit test that verifies facadeInstance is non-null after create() returns, to guard against regressions.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/services/auto-mode/facade.ts` around lines 146 - 159, Add a
unit test to guard the deferred facade binding: instantiate the module that
calls create() (the factory that assigns facadeInstance) and assert that after
calling create() the closure-backed accessor getFacade() returns a non-null
AutoModeServiceFacade (and does not throw). Target the public factory/function
that performs the creation (the create() call that sets facadeInstance) and
exercise the callbacks passed to
PipelineOrchestrator/AutoLoopCoordinator/ExecutionService asynchronously to
ensure the invariant holds; fail the test if getFacade() throws or returns null.

apps/server/src/services/agent-executor.ts (2)

518-520: Potential type confusion: .length used as boolean without explicit comparison.

hasFeedback and hasEdits are assigned numeric values (from .length) or undefined, then used in boolean context. While this works in JS/TS due to truthiness, the variable names suggest boolean intent. This is minor but could confuse future readers.

Optional: make intent explicit

-          const hasFeedback = approvalResult.feedback?.trim().length,
-            hasEdits = approvalResult.editedPlan?.trim().length;
+          const hasFeedback = (approvalResult.feedback?.trim().length ?? 0) > 0;
+          const hasEdits = (approvalResult.editedPlan?.trim().length ?? 0) > 0;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/services/agent-executor.ts` around lines 518 - 520, The
variables hasFeedback and hasEdits are currently set to `.length` (a number or
undefined) which relies on truthiness; change them to explicit booleans to match
their names — e.g., set hasFeedback =
Boolean(approvalResult.feedback?.trim().length) or hasFeedback =
(approvalResult.feedback?.trim().length ?? 0) > 0 and likewise for hasEdits —
then keep the existing conditional that throws the Error('Plan cancelled by
user') when both are false; update the expressions near approvalResult.feedback
and approvalResult.editedPlan to use the chosen explicit boolean form.

---

52-276: The execute method is ~225 lines long with deeply nested logic.

This method handles streaming, spec detection, plan approval branching, file I/O scheduling, heartbeats, and raw output — all in one function. While functional, the high cyclomatic complexity makes it harder to test and maintain. Consider extracting the stream-processing loop (lines 182-260) and the file-write setup (lines 114-143) into focused private methods in a future pass.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/services/agent-executor.ts` around lines 52 - 276, The
execute method is too large and mixes concerns; extract the file-write setup and
the stream-processing loop into private helper methods to reduce cyclomatic
complexity. Move the writeToFile, scheduleWrite, and appendRawEvent logic into a
private method (e.g., setupFileWriters or flushHelpers) that returns the helper
functions and cleanup handles; extract the for-await-of provider.executeQuery
loop body (the streaming logic that handles msg types, text/tool blocks, spec
detection and calls to handleSpecGenerated/executeTasksLoop) into a private
method (e.g., processStreamMessages) that accepts the stream iterator, options,
callbacks, and mutable state (responseText, specDetected, tasksCompleted,
aborted) and returns the updated state; keep execute as the orchestration layer
that calls setupFileWriters, then processStreamMessages, handles cleanup
(clearing timers, final writes) and returns the result.

apps/server/src/services/pipeline-orchestrator.ts (1)

56-70: Constructor takes 12 parameters — consider a config/options object.

The constructor has grown to accept 12 dependencies. While this is common for orchestrators, it reduces readability and makes instantiation (especially in tests) cumbersome. A grouped options/deps object would improve ergonomics.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/services/pipeline-orchestrator.ts` around lines 56 - 70, The
PipelineOrchestrator constructor currently accepts 12 positional dependencies
(TypedEventBus, FeatureStateManager, AgentExecutor, TestRunnerService,
WorktreeResolver, ConcurrencyManager, SettingsService|null,
UpdateFeatureStatusFn, loadContextFiles, BuildFeaturePromptFn, ExecuteFeatureFn,
RunAgentFn), which is unwieldy; refactor the constructor to accept a single
options/deps object (e.g. type PipelineOrchestratorOptions) that contains those
named properties, update the class to destructure that options object in the
constructor, and then update all call sites and tests that instantiate
PipelineOrchestrator to pass a named object instead of a long positional
argument list, preserving existing types and nullability for SettingsService and
keeping the same property names as in the original constructor.